Closed
Bug 1472087
Opened 7 years ago
Closed 7 years ago
DeCOMtaminate nsIDocShellLoadInfo
Categories
(Core :: DOM: Navigation, enhancement, P2)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: qdot, Assigned: qdot)
References
(Blocks 1 open bug)
Details
Attachments
(6 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
nika
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
nika
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
nika
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
nika
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
nika
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
nika
:
review+
|
Details |
nsIDocShellLoadInfo isn't really used between JS and C++ (the only IDL references are as parameters, and the only JS use is of the loadHistory type), so we should be able to consolidate it to C++ only.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b558e4dcdd75e98e75ebb1a79795566bc54fbe38
Cleanup left:
- Turn nsDocShellInfoLoadType into its own enum in nsDocShellLoadTypes
- Turn nsDocShellInfoReferrerPolicy into its own enum in nsDocShellLoadTypes
- Change nsDocShellLoadInfo getters/setters to webidl style instead of XPCOM style
Updated•7 years ago
|
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
Comment on attachment 8989506 [details]
Bug ? - Remove redundant Docshell destroyed check;
Wrong patch, not supposed to be in this set.
Attachment #8989506 -
Attachment is obsolete: true
Attachment #8989506 -
Flags: review?(nika)
Assignee | ||
Updated•7 years ago
|
Attachment #8989506 -
Attachment is obsolete: false
Assignee | ||
Updated•7 years ago
|
Attachment #8989506 -
Attachment is obsolete: true
Assignee | ||
Comment 13•7 years ago
|
||
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8988970 [details]
Bug 1472087 - deCOMtaminate nsIDocShellLoadInfo;
https://reviewboard.mozilla.org/r/254084/#review261634
::: docshell/base/nsDocShell.cpp:713
(Diff revision 2)
> if (aLoadInfo) {
> aLoadInfo->GetReferrer(getter_AddRefs(referrer));
> aLoadInfo->GetOriginalURI(getter_AddRefs(originalURI));
> GetMaybeResultPrincipalURI(aLoadInfo, resultPrincipalURI);
> aLoadInfo->GetLoadReplace(&loadReplace);
> - nsDocShellInfoLoadType lt = nsIDocShellLoadInfo::loadNormal;
> + nsDocShellLoadInfo::nsDocShellInfoLoadType lt = nsDocShellLoadInfo::loadNormal;
I think that, given this type is namespaced now, we can probably get away with calling it just 'LoadType'.
::: docshell/base/nsDocShellLoadInfo.h:23
(Diff revision 2)
>
> -class nsDocShellLoadInfo : public nsIDocShellLoadInfo
> +class nsDocShellLoadInfo
> {
> public:
> + typedef uint32_t nsDocShellInfoLoadType;
> + typedef uint32_t nsDocShellInfoReferrerPolicy;
both of these can probably be given more terse names, but come to think of it you might do that in one of the other... 6? patches.
::: toolkit/components/viewsource/content/viewSource-content.js:268
(Diff revision 2)
> .createInstance(Ci.nsISHEntry);
> shEntry.setURI(Services.io.newURI(viewSrcURL));
> shEntry.setTitle(viewSrcURL);
> let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
> shEntry.triggeringPrincipal = systemPrincipal;
> - shEntry.loadType = Ci.nsIDocShellLoadInfo.loadHistory;
> + shEntry.loadType = 2; /* nsDocShellLoadInfo::loadHistory */
:-/ - Thanks for adding the comment there at least!
::: toolkit/modules/sessionstore/SessionHistory.jsm:346
(Diff revision 2)
>
> shEntry.setURI(Utils.makeURI(entry.url));
> shEntry.setTitle(entry.title || entry.url);
> if (entry.subframe)
> shEntry.setIsSubFrame(entry.subframe || false);
> - shEntry.loadType = Ci.nsIDocShellLoadInfo.loadHistory;
> + shEntry.loadType = 2; /* nsDocShellLoadInfo::loadHistory */
:-/
Attachment #8988970 -
Flags: review?(nika) → review+
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8988971 [details]
Bug 1472087 - Remove nsIDocShellLoadInfo.idl;
https://reviewboard.mozilla.org/r/254086/#review261636
Attachment #8988971 -
Flags: review?(nika) → review+
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8988972 [details]
Bug 1472087 - Remove nsDocShell::CreateLoadInfo;
https://reviewboard.mozilla.org/r/254088/#review261638
Attachment #8988972 -
Flags: review?(nika) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8989507 [details]
Bug 1472087 - Convert nsDocShellLoadInfo to WebIDL style;
https://reviewboard.mozilla.org/r/254542/#review261640
::: commit-message-1b750:4
(Diff revision 1)
> +Bug 1472087 - Convert nsDocShellLoadInfo to WebIDL style; r?nika
> +
> +While nsDocShellLoadInfo isn't represented by WebIDL (because we don't
> +need it in JS currently), make the getter/setter interface look
Is the plan to eventually also expose nsDocShellLoadInfo to JS so that we can convert all load entry points in nsDocShell to using it?
::: docshell/base/nsDocShellLoadInfo.h:22
(Diff revision 1)
> class nsIDocShell;
>
> class nsDocShellLoadInfo
> {
> public:
> typedef uint32_t nsDocShellInfoLoadType;
Can we kill this typedef? We have an enum type now.
::: docshell/base/nsDocShellLoadInfo.h:157
(Diff revision 1)
> bool mInheritPrincipal;
> bool mPrincipalIsExplicit;
> bool mForceAllowDataURI;
> bool mOriginalFrameSrc;
> bool mSendReferrer;
> - nsDocShellInfoReferrerPolicy mReferrerPolicy;
> + uint32_t mReferrerPolicy;
Could/should we use the 'mozilla::net::ReferrerPolicy' enum for this to make it more clear?
::: docshell/base/nsDocShellLoadInfo.cpp:159
(Diff revision 1)
> {
> mOriginalFrameSrc = aOriginalFrameSrc;
> - return NS_OK;
> }
>
> -NS_IMETHODIMP
> +nsDocShellLoadInfo::nsDocShellInfoLoadType
Can we make this nsDocShellLoadInfo::LoadType now - we have the enum :-)
::: docshell/base/nsDocShellLoadInfo.cpp:166
(Diff revision 1)
> - *aLoadType = mLoadType;
> - return NS_OK;
> }
>
> -NS_IMETHODIMP
> -nsDocShellLoadInfo::SetLoadType(nsDocShellInfoLoadType aLoadType)
> +void
> +nsDocShellLoadInfo::SetLoadType(nsDocShellLoadInfo::nsDocShellInfoLoadType aLoadType)
Same here
Attachment #8989507 -
Flags: review?(nika) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8989508 [details]
Bug 1472087 - Remove nsDocShellLoadInfo::LoadTypes;
https://reviewboard.mozilla.org/r/254544/#review261646
::: toolkit/components/viewsource/content/viewSource-content.js:268
(Diff revision 1)
> .createInstance(Ci.nsISHEntry);
> shEntry.setURI(Services.io.newURI(viewSrcURL));
> shEntry.setTitle(viewSrcURL);
> let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
> shEntry.triggeringPrincipal = systemPrincipal;
> - shEntry.loadType = 2; /* nsDocShellLoadInfo::loadHistory */
> + shEntry.loadType = 4; /* nsDocShellLoadTypes LOAD_HISTORY */
This is a touch more nasty now :-/...
Maybe even add a method to nsISHistory like setAsHistoryLoad()? D:
Attachment #8989508 -
Flags: review?(nika) → review+
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8989509 [details]
Bug 1472087 - Readd comments from nsIDocShellLoadInfo;
https://reviewboard.mozilla.org/r/254546/#review261648
::: docshell/base/nsDocShellLoadInfo.h:21
(Diff revision 1)
> class nsISHEntry;
> class nsIURI;
> class nsIDocShell;
>
> +/**
> + * nsDocShellLoadInfo defines an interface for specifying setup information used
nsDocShellLoadInfo isn't really an interface anymore :-P
Attachment #8989509 -
Flags: review?(nika) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•7 years ago
|
||
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9df2dcfe09f
deCOMtaminate nsIDocShellLoadInfo; r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffbed43e8c35
Remove nsIDocShellLoadInfo.idl; r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/74c808bf7da8
Remove nsDocShell::CreateLoadInfo; r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0289eb6a02d
Convert nsDocShellLoadInfo to WebIDL style; r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd86e6f12106
Remove nsDocShellLoadInfo::LoadTypes; r=nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d9d71567046
Readd comments from nsIDocShellLoadInfo; r=nika
Comment 30•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e9df2dcfe09f
https://hg.mozilla.org/mozilla-central/rev/ffbed43e8c35
https://hg.mozilla.org/mozilla-central/rev/74c808bf7da8
https://hg.mozilla.org/mozilla-central/rev/f0289eb6a02d
https://hg.mozilla.org/mozilla-central/rev/fd86e6f12106
https://hg.mozilla.org/mozilla-central/rev/8d9d71567046
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•